Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(token-search): fix token search results #3607

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

alfetopito
Copy link
Collaborator

Summary

Fixes #3599

There were 2 issues with token search

  1. Sorting inconsistency

Due to default browsers' sorting behaviour, the results were different when the symbols were identical.
This is the case for StaFi and Rocket Poll Eth.

  1. Only last match was picked

In the displayed list of matches, we picked the one matching exactly the search terms, by address or symbol.
Turns out symbol is not unique, so we were silently dropping everything but the last match.
This is the case for both tokens above, as well as COW Protocol Token and cowfarm.finance

To Test

  1. Using firefox, on mainnet, search for reth
  • Should show 3 active token matches plus other inactive/additional matches
    image
  1. Search for cow
  • Should show 1 active token match plus other inactive/additional matches
    image
  1. Repeat 1 and 2 using chrome/brave/safari

@alfetopito alfetopito self-assigned this Jan 10, 2024
Copy link

vercel bot commented Jan 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Jan 11, 2024 1:50pm
explorer ✅ Ready (Inspect) Visit Preview Jan 11, 2024 1:50pm
swap-dev ✅ Ready (Inspect) Visit Preview Jan 11, 2024 1:50pm
widget-configurator ✅ Ready (Inspect) Visit Preview Jan 11, 2024 1:50pm

@alfetopito alfetopito requested review from elena-zh and a team January 10, 2024 17:29
Comment on lines -64 to +66
matched = t
// There should ever be only 1 token with a given address
// There can be multiple with the same symbol
matched.push(t)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for missing entries.
Now all matches are collected.

@@ -7,6 +7,6 @@ import { TokensMap } from '../types'
*/
export function tokenMapToListWithLogo(tokenMap: TokensMap): TokenWithLogo[] {
return Object.values(tokenMap)
.sort((a, b) => (a.symbol > b.symbol ? 1 : -1))
.sort((a, b) => (a.symbol === b.symbol ? 0 : a.symbol > b.symbol ? 1 : -1))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for sorting.
We were not accounting for when symbols are identical.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups we missed this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, localeCompare is what we should use for strings.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! I didn't even know about it

<h4>Favourite tokens</h4>
<InfoIcon iconType="help" content="Your favourite saved tokens. Edit this list in your account page." />
<h4>Favorite tokens</h4>
<InfoIcon iconType="help" content="Your favorite saved tokens. Edit this list in your account page." />
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional request by Alex to update the spelling.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the 🕵️ and fix!

@@ -7,6 +7,6 @@ import { TokensMap } from '../types'
*/
export function tokenMapToListWithLogo(tokenMap: TokensMap): TokenWithLogo[] {
return Object.values(tokenMap)
.sort((a, b) => (a.symbol > b.symbol ? 1 : -1))
.sort((a, b) => (a.symbol === b.symbol ? 0 : a.symbol > b.symbol ? 1 : -1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups we missed this!

@@ -7,6 +7,6 @@ import { TokensMap } from '../types'
*/
export function tokenMapToListWithLogo(tokenMap: TokensMap): TokenWithLogo[] {
return Object.values(tokenMap)
.sort((a, b) => (a.symbol > b.symbol ? 1 : -1))
.sort((a, b) => a.symbol.localeCompare(b.symbol))
Copy link
Contributor

@anxolin anxolin Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funny, i just came to suggest this (use localeCompare), and I saw it was done moments ago!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@shoom3301 shoom3301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great!

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent token search results across browsers
4 participants